7181: Loading Compressor using ClassLoader configured through setServiceClassLoader#7428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7428 +/- ##
============================================
+ Coverage 89.75% 89.78% +0.02%
- Complexity 6980 6998 +18
============================================
Files 797 798 +1
Lines 21165 21204 +39
Branches 2057 2058 +1
============================================
+ Hits 18996 19037 +41
+ Misses 1505 1503 -2
Partials 664 664 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| requireNonNull(compressionMethod, "compressionMethod"); | ||
| Compressor compressor = CompressorUtil.validateAndResolveCompressor(compressionMethod); | ||
| Compressor compressor = | ||
| CompressorUtil.validateAndResolveCompressor(compressionMethod, serviceClassLoader); |
There was a problem hiding this comment.
Seems like we should move the call to CompressorUtil.validateAndResolveCompressor down to HttpExporterBuilder, GrpcExporterBuilder to avoid the clutter of keeping ClassLoader serviceClassLoader property.
There was a problem hiding this comment.
Thanks for reviewing this. I agree with you and I am making the changes. Let me get back after wrap them up.
There was a problem hiding this comment.
@jack-berg Let me know how the current version looks.
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Just one recommendation to declutter, but looks good. Thanks for working on this.
See also #7446 - after seeing this PR I realized that adding this logic still wouldn't allow autoconfigure users to be able to specify the class loader used to load the compressor. With this and #7446 we have a good holistic story.
…porters as per feedback
2 participants
Context
This PR aims to resolve ClassLoader issue from this bug ticket.